-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
PEP 796: Relative Virtual Environments #4476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Hello, before we go any further with this PR, has the idea been discussed on Discourse? And after that we'll need a sponsor before assigning the PEP number, do you have one yet? Let's unassign 796 for now. Please see: |
Thanks @hugovk and @StanFromIreland for the early review. I used a draft PR to see the CI results to further clean it up -- my apologies for wasting some of your time, but thank you regardless.
Yes: https://discuss.python.org/t/making-venvs-relocatable-friendly/96177 The criteria of "discussed enough with enough support" is vague, but what gave me enough confidence to begin a PEP and start a (draft) PR at this point was:
Thanks for clarifying that part! Yes, finding a sponsor is my next big step. |
Tip: you can enable GitHub Actions at https://github.com/rickeylev/peps/actions and run the CI on your fork.
Good luck! |
This comment has been minimized.
This comment has been minimized.
@paveldikov thank you for the comment, please could you re-post it on Discourse? The peps repo / PR discussion is mainly for editorial discussion, rather than substantive comment on the proposal itself. A |
Co-authored-by: Hugo van Kemenade <[email protected]> Co-authored-by: Zanie Blue <[email protected]>
…to relative.venvs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reviews! I'm out from under $dayJob and vacation backlogs a bit now and addressed comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some little notes
Belatedly removed the DO-NOT-MERGE label (that was added pending the Discourse discussion and sponsorship of the PEP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PEP is currently missing a Security Implications section, which I think should be added given that arbitrary directory traversal is permitted -- at the very least explaining why this is fine.
Several editorial notes, I think the Motivation & Rationale sections should be strengthened to focus on the benefits from relative environments, there is currently (I believe) a lot of assumed context.
The PEP also discusses at some length a broader proposal for reloacatable venvs. Is it worth considering making that the proposal here? I don't know the specifics, so it might be that the changes needed for 'relocatable' are too large to tackle in one go.
A
Co-authored-by: Alyssa Coghlan <[email protected]> Co-authored-by: Adam Turner <[email protected]> Co-authored-by: Stan Ulbrych <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied suggestions and addressed a few comments; didn't have time to address everything, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, all pending comments addressed (I think. The Github UI frequently tricks me 😂 )
PTAL
re: security section: added. At least, insofar as I understand your concern correctly.
re: motivation and rationale strengthening: Thanks! Yeah, honestly, a big issue I've had writing this that "Motivation" and "Rationale" are, to me at least, practically synonyms. I did my best to better separate "Why do this change at all?" (motivation) and "Implementation decision and why that decision" (rationale).
re: proposing relocatable venv in this pep: I added a section about why this pep limits its scope to just the core interpreter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few editorial comments inline, but I think this is essentially ready for publication.
code in that virtual environment. Currently, this path is required to be | ||
absolute for correct virtual environment operation because the original | ||
`PEP 405 <https://peps.python.org/pep-0405/>`__ | ||
specifying virtual environments didn't cover any specific way of processing | ||
relative paths, their behaviour is implementation dependent. CPython releases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusting a run-on sentence:
code in that virtual environment. Currently, this path is required to be | |
absolute for correct virtual environment operation because the original | |
`PEP 405 <https://peps.python.org/pep-0405/>`__ | |
specifying virtual environments didn't cover any specific way of processing | |
relative paths, their behaviour is implementation dependent. CPython releases | |
code in that virtual environment. Currently, this path is required to be | |
absolute for consistent virtual environment operation (the original | |
`PEP 405 <https://peps.python.org/pep-0405/>`__ | |
specifying virtual environments didn't cover any specific way of processing | |
relative paths, so their behaviour is implementation dependent). CPython releases |
Portable virtual environments are important for the efficiency and | ||
reproducibility benefits they bring from being created once and reused multiple | ||
times later in different locations. For example, a build farm can build a | ||
virtual environment once, cache it, and then re-use it as-is to CI jobs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual environment once, cache it, and then re-use it as-is to CI jobs. | |
virtual environment once, cache it, and then re-use it as-is to run CI jobs. |
Fullying designing portable virtual environments | ||
------------------------------------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fullying designing portable virtual environments | |
------------------------------------------------ | |
Deferring the design of fully portable virtual environments | |
----------------------------------------------------------- |
|
||
This PEP purposely only focuses on the interpreter startup behavior to limit | ||
its scope. There are multiple implementations and many design questions for how | ||
to implement portable virtual environments work (e.g. what installers should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to implement portable virtual environments work (e.g. what installers should | |
to fully implement portable virtual environments (e.g. what installers should |
During interpreter startup (i.e. :file:`getpath.py`), the relative path is joined to the | ||
directory containing ``pyvenv.cfg`` to form an absolute path. | ||
Parent-directory references (``../``) and current | ||
directory references (``./``) are resolved syntactically (i.e. not resolving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directory references (``./``) are resolved syntactically (i.e. not resolving | |
directory references (``./``) are resolved lexically (i.e. not resolving |
(I don't mind if this isn't accepted, I've just always referred to this style of resolution as lexical rather than as syntactic: python/cpython#124825
|
||
Currently, relative paths resolve relative to the process's current working | ||
directory. Because the current working directory isn't knowable in advance, it | ||
makes relative paths today effectively impossible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes relative paths today effectively impossible. | |
makes using relative paths today effectively impossible. |
|
||
This PEP does not specify how to create a ``pyvenv.cfg`` with a relative path, | ||
nor how downstream tools (e.g. installers) should identify them or process | ||
them. These questions are best addressed separately by tool owners. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't an open issue, but rather than intentional omission. Perhaps just include a variant of this paragraph in the rationale subsection about not fully specifying portable virtual environments?
determine the actual Python interpreter installation that is used to execute | ||
code in that virtual environment. Currently, this path is required to be | ||
absolute for correct virtual environment operation because the original | ||
`PEP 405 <https://peps.python.org/pep-0405/>`__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint fix:
`PEP 405 <https://peps.python.org/pep-0405/>`__ | |
:pep:`405` |
machines do so either by relying on undocumented interpreter behaviour | ||
(Bazel, omitting the ``home`` key entirely to trigger an implementation | ||
dependent fallback to resolving via a symlinked interpreter binary on | ||
non-Windows systems, see `gh-135773`) or by requiring a post-installation script to be executed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint fix:
Alyssa's suggestion below is better.
``PYTHONHOME``. | ||
A portable virtual environment is one that can be moved between | ||
platform-compatible hosts, which is an important feature for some projects (see | ||
"Why portable environments matter"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can link it:
"Why portable environments matter"). | |
`Why portable virtual environments matter`_). |
|
||
The reason to support a relative path is to support portable virtual | ||
environments, which rely on using a host-agnostic relative path to point to | ||
``PYTHONHOME``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a role for env vars as well, which will link them to the docs:
``PYTHONHOME``. | |
:envvar:`PYTHONHOME`. |
Can also update the other instances below if you like.
Re: https://devguide.python.org/documentation/markup/#information-units
after the environment is placed in its target location ( | ||
`venvstacks <https://lmstudio.ai/blog/venvstacks#publishing-environment-layer-archives>`__ | ||
). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove spaces from inside parentheses:
after the environment is placed in its target location ( | |
`venvstacks <https://lmstudio.ai/blog/venvstacks#publishing-environment-layer-archives>`__ | |
). | |
after the environment is placed in its target location (`venvstacks | |
<https://lmstudio.ai/blog/venvstacks#publishing-environment-layer-archives>`__). |
How to Teach This | ||
================= | ||
|
||
Teaching this should be straightforward: if you use a relative path in | ||
``pyvenv.cfg``, then it's relative to the directory containing the | ||
``pyvenv.cfg`` file. This is simple to explain and easy to understand for | ||
anyone that is already familiar with handling relative filesystem paths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be useful to say where you will teach this. What docs will you update?
Python runtime. | ||
* `PoC for relative home in Python startup <https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__ | ||
* `Python Ideas "Making venvs relocatable friendly" discussion <https://discuss.python.org/t/making-venvs-relocatable-friendly/96177>`__ | ||
* `gh-136051: relative pyvenv.cfg home <https://github.com/python/cpython/issues/136051>`__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed now there's an inline link above.
* `gh-136051: relative pyvenv.cfg home <https://github.com/python/cpython/issues/136051>`__ |
paths. | ||
|
||
A proof-of-concept of this is implemented in the author's branch, | ||
`rickeylev/feat.relative.pyvenv.home <https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`rickeylev/feat.relative.pyvenv.home <https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__. | |
`rickeylev/feat.relative.pyvenv.home | |
<https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__. |
Please could you also wrap some of the other long lines to 79 chars?
* `venvstacks <https://pypi.org/project/venvstacks/>`__: a tool for creating | ||
reproducible distribution artifacts from virtual environments A relocatable | ||
Python runtime. | ||
* `PoC for relative home in Python startup <https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already linked in the reference implementation section.
* `PoC for relative home in Python startup <https://github.com/python/cpython/compare/main...rickeylev:cpython:feat.relative.pyvenv.home>`__ |
host-relocatable virtual environments. | ||
* `python-build-standalone <https://github.com/astral-sh/python-build-standalone>`__ | ||
* `venvstacks <https://pypi.org/project/venvstacks/>`__: a tool for creating | ||
reproducible distribution artifacts from virtual environments A relocatable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reproducible distribution artifacts from virtual environments A relocatable | |
reproducible distribution artifacts from virtual environments. A relocatable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are the two items the PEP linter is picking up.
absolute for correct virtual environment operation because the original | ||
`PEP 405 <https://peps.python.org/pep-0405/>`__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolute for correct virtual environment operation because the original | |
`PEP 405 <https://peps.python.org/pep-0405/>`__ | |
absolute for correct virtual environment operation because the original :pep:`405` |
machines do so either by relying on undocumented interpreter behaviour | ||
(Bazel, omitting the ``home`` key entirely to trigger an implementation | ||
dependent fallback to resolving via a symlinked interpreter binary on | ||
non-Windows systems, see `gh-135773`) or by requiring a post-installation script to be executed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-Windows systems, see `gh-135773`) or by requiring a post-installation script to be executed | |
non-Windows systems, see :cpython-issue:`135773`) or by requiring a post-installation script to be executed |
Basic requirements (all PEP Types)
pep-NNNN.rst
), PR title (PEP 123: <Title of PEP>
) andPEP
headerAuthor
orSponsor
, and formally confirmed their approvalAuthor
,Status
(Draft
),Type
andCreated
headers filled out correctlyPEP-Delegate
,Topic
,Requires
andReplaces
headers completed if appropriate.github/CODEOWNERS
for the PEPStandards Track requirements
Python-Version
set to valid (pre-beta) future Python version, if relevantDiscussions-To
andPost-History
Work towards python/cpython#136051
📚 Documentation preview 📚: https://pep-previews--4476.org.readthedocs.build/